Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(codegen-ui-react): prevent useless props forward to custom components #1009

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JJK801
Copy link

@JJK801 JJK801 commented May 12, 2023

Problem

When we create composite components (components with nested components) in Figma and they go through the studio and amplify-codegen-ui, any props are forwarded to the nested component (including the defaults that were not configured in the studio).

When using variants with logic inside the components (breakpoint, displayMode, ...), this breaks the logic because the forwarded prop will always be the default variant.

Example:

GIVEN i created a NavBar component in Figma with 2 breakpoint variants (small, medium)
GIVEN i nested my NavBar component in a PageLayout component in Figma
WHEN codegen-amplify-ui generates my PageLayout component
THEN the nested NavBar component will receive a breakpoint="small" prop from PageLayout
THEN the forwarded prop overrides the breakpoint value generated inside the NavBar component

Solution

Studio produced JSON includes a configured key for props added via the studio, so we can use it to filter custom components props. Implemented forwarding filter rule is the following:

  • Component IS a Primitive => Because Primitives needs explicit props forwarding
  • Component IS a HTML tag => Because HTML tags needs explicit props forwarding
  • Prop value is dynamic (prop key is in localStateReferences)
  • Prop value is a Collection binding
  • Prop value is a concat
  • Prop value has been configured in the studio

Additional Notes

I fixed most of the specs by simply adding the configured prop to the dataset. It seems the best way to do, but pay attention so that doesn't change the nature of the test.

Links

Ticket

Other links

Verification

Manual tests

Automated tests

  • Unit tests added/updated
  • E2E tests added/updated
  • N/A - (provide a reason)
  • deferred - (provide GitHub issue for tracking)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@JJK801 JJK801 requested a review from a team as a code owner May 12, 2023 13:46
@codecov-commenter
Copy link

Codecov Report

Merging #1009 (208bbf6) into main (b3f340a) will increase coverage by 12.45%.
The diff coverage is 94.32%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #1009       +/-   ##
===========================================
+ Coverage   81.26%   93.71%   +12.45%     
===========================================
  Files         121      122        +1     
  Lines        5369     5428       +59     
  Branches     1599     1620       +21     
===========================================
+ Hits         4363     5087      +724     
+ Misses        935      323      -612     
+ Partials       71       18       -53     
Impacted Files Coverage Δ
...react/lib/forms/form-renderer-helper/form-state.ts 98.84% <ø> (+23.98%) ⬆️
...b/forms/form-renderer-helper/render-array-field.ts 100.00% <ø> (+86.76%) ⬆️
.../codegen-ui-react/lib/forms/react-form-renderer.ts 96.46% <ø> (+25.61%) ⬆️
...ges/codegen-ui/lib/required-dependency-provider.ts 0.00% <ø> (ø)
...ages/codegen-ui-react/lib/utils/storage-manager.ts 22.22% <22.22%> (ø)
...m-to-component/map-form-definition-to-component.ts 88.46% <75.00%> (-2.85%) ⬇️
.../forms/form-renderer-helper/event-handler-props.ts 99.13% <100.00%> (+36.03%) ⬆️
...ct/lib/forms/form-renderer-helper/event-targets.ts 100.00% <100.00%> (+65.11%) ⬆️
...act/lib/forms/form-renderer-helper/model-values.ts 91.97% <100.00%> (+72.22%) ⬆️
.../lib/forms/form-renderer-helper/render-checkers.ts 100.00% <100.00%> (ø)
... and 10 more

... and 19 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e23c0c...208bbf6. Read the comment docs.

@JJK801
Copy link
Author

JJK801 commented May 23, 2023

Hi 🖖

Any news about this PR ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants